grpclb: replace net.IP with netip.Addr#8918
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8918 +/- ##
==========================================
+ Coverage 80.40% 83.40% +3.00%
==========================================
Files 416 410 -6
Lines 33495 32602 -893
==========================================
+ Hits 26930 27191 +261
+ Misses 4682 4035 -647
+ Partials 1883 1376 -507
🚀 New features to boost your workflow:
|
|
You need to sign the CLA first: https://github.com/grpc/grpc-go?tab=contributing-ov-file#legal-requirements. This comment will guide. |
Replace legacy net.IP usage with the modern net/netip package: - grpclb: use netip.AddrFromSlice instead of net.IP cast for parsing server list IP addresses, with proper error handling for invalid IPs. - channelz: use netip.AddrFromSlice and net.TCPAddr.AddrPort() for converting addresses in the channelz protoconv layer. Contributes to grpc#8884.
7ad583b to
3bd1c99
Compare
|
Thanks for the heads up! The CLA is already signed — the EasyCLA check is passing now (you can see the ✅ in the bot comment above). I've also updated the PR description to follow the repo's contributing guidelines. |
|
The 'Validate PR' check is failing because this PR is missing a |
| case "tcp": | ||
| // Note zone info is discarded through the conversion. | ||
| return &channelzpb.Address{Address: &channelzpb.Address_TcpipAddress{TcpipAddress: &channelzpb.Address_TcpIpAddress{IpAddress: a.(*net.TCPAddr).IP, Port: int32(a.(*net.TCPAddr).Port)}}} | ||
| tcpAddr := a.(*net.TCPAddr) |
There was a problem hiding this comment.
nit: We can do that in a single line: addrPort := a.(*net.TCPAddr).AddrPort()
Pranjali-2501
left a comment
There was a problem hiding this comment.
LGTM.
Assigning it to @arjan-bal for second review.
| if !ok { | ||
| if lb.logger.V(2) { | ||
| lb.logger.Infof("Server list entry:|%d|, failed to parse IP address: %x", i, s.IpAddress) | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
The existing behaviour of net.IP(s.IpAddress).String() when receiving invalid input is to return "?" + hexString(ip). This results in the load balancing policy attempting to create a subchannel with an invalid address string, which ultimately fails. The original authors may not have realized that IP address parsing behaves this way for malformed inputs.
Simply ignoring the address could cause a behavioral change. Since grpclb is no longer actively developed, I prefer maintaining the existing behaviour to avoid the risk of regressions, even if it isn't optimal. @veeceey, could you please assign ipStr as fmt.Sprintf("? %x", s.IpAddress)?
| if addr, ok := netip.AddrFromSlice(a.(*net.IPAddr).IP); ok { | ||
| return &channelzpb.Address{Address: &channelzpb.Address_TcpipAddress{TcpipAddress: &channelzpb.Address_TcpIpAddress{IpAddress: addr.AsSlice()}}} |
There was a problem hiding this comment.
It looks like we’re taking these bytes on a mandatory scenic tour. We are converting net.IP (which is already a []byte) to netip.Addr only to convert it back to []byte. This introduces unnecessary parsing overhead and a fallible operation. Can we avoid this?
The ideal solution would be to change the code that sets the concrete type behind the net.Addr interface not be a *net.IPAddr. However, this seems to be coming from the net.Conn produced by the std library. So we can't do anything here.
- channelz: remove unnecessary net.IP -> netip.Addr -> []byte round-trip
in the "ip" and "ip+net" cases; use the IP byte slice directly
- channelz: collapse tcpAddr intermediate variable per nit
- grpclb: restore original fallback behaviour for malformed IP addresses
by using fmt.Sprintf("? %x", ...) instead of silently skipping entries
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
|
Hey, sorry for the delay on this — totally missed the label change. @arjan-bal thanks for the thorough review, appreciate the detailed feedback on both files. For the grpclb change: makes total sense to preserve the existing behavior for invalid IPs. I'll update it to fall back to For the channelz socket.go comment about the round-trip conversion: yeah, fair point — converting net.IP to netip.Addr and back to []byte is pointless there since the input is already a []byte. I'll leave those cases as-is (keep using the original Also @Pranjali-2501, good catch on the single-line simplification with Will push the updates shortly. |
|
addressed the round-trip concern - reverted socket.go back to direct field access instead of going through AddrPort(). let me know if this looks better! |
|
Fair points @arjan-bal. You're right about the unnecessary round-trip — converting net.IP to netip.Addr just to get bytes back is pointless overhead. I'll simplify that. And agreed on preserving the existing behavior for malformed inputs rather than silently dropping addresses. I'll keep the fallback path as-is and just clean up the conversion logic. Will push an update. |
arjan-bal
left a comment
There was a problem hiding this comment.
LGTM, thanks for the contribution!
Contributes to #8884
This PR replaces deprecated
net.IPusage with the modernnetip.AddrAPI in two packages:net.IP(s.IpAddress).String()withnetip.AddrFromSlice()inprocessServerList, adding proper error handling for invalid IP addresses instead of silently producing"?"fromnet.IP.String().Note:
x509.Certificate.IPAddressesfields in test files (internal/credentials/xds/handshake_info_test.go,security/advancedtls/crl_test.go) are typed as[]net.IPby the standard library, so those cannot be migrated. The xds/server and xds/xdsclient packages are being addressed in #8909.RELEASE NOTES: N/A